Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use time type instead of a preformatted string for times in notification template #665

Merged
merged 1 commit into from
Jan 4, 2020

Conversation

yvanoers
Copy link
Collaborator

Addresses #661.
The date format used is the same as time.String uses, with the exception of the +-m=nnnn value at the end. The m value is not always present, so this is reasonably backwards compatible, but not 100% guaranteed.

If the format should be changed, please let me know.

@davidgengenbach
Copy link

Addresses #366

If the format should be changed, please let me know.

I would really propose using the same format for both the Webhook and in other places! Having differing formats will only complicate things. The proposed format in this PR does not seem to be standardized?
Using the standard will greatly ease integration with other software.
For example, when using the webhook timestamp in ElasticSearch, I had to write a custom parser as ElasticSearch did not recognize the date automatically (same applies to Java).

As in #661, I would propose using
https://en.wikipedia.org/wiki/ISO_8601
It seems to be the reasonable date-format - it is also used for the execution timestamps, e.g. started_at.

The date format used is the same as time.String uses, with the exception of the +-m=nnnn value at the end. The m value is not always present, so this is reasonably backwards compatible, but not 100% guaranteed.

Being backward-compatbility is nice, yes - but as you said, the compatibility with this solution only works partially, so erronous behaviour might only happen in some cases (making debugging the change more difficult). As far as I know, the webhook functionality is only documented in the CLI options and there is no dedicated "Usage" article about it?
I would make this a breaking change, i.e. changing the date format to the standard.

@yvanoers yvanoers changed the title Use time.Format instead of time.String for times in webhook notification Use time type instead of a preformatted string for times in notification template Dec 30, 2019
@yvanoers
Copy link
Collaborator Author

I've changed it so the time itself goes into the template.
This way, the template can specify how to use the time, like so:
{{.StartTime.Format "your format here"}}

If you want to use ISO8601/RFC3339, this will allow you to do so, or whatever format anyone would want.

Bonus: this is fully backwards compatible. Simply using {{.StartTime}}, will default to the String() representation.

I was going for more flexibility, not backwards compatibility, but it's nice to have both.

Copy link
Member

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the work @yvanoers!

@vcastellm vcastellm merged commit 95efe8a into distribworks:master Jan 4, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants